Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle ResponseErrors correctly #416

Merged
merged 1 commit into from
Nov 26, 2023
Merged

handle ResponseErrors correctly #416

merged 1 commit into from
Nov 26, 2023

Conversation

dimbleby
Copy link
Contributor

The error on a ResponseErrorMessage is supposed to be a ResponseError, not a dictionary.

Older versions of cattrs seem to have tolerated the mistake, but newer ones don't.

The reason that this repository's own tests don't find this is that it continues to support python 3.7: and the versions of cattrs that don't like the mistake have dropped support for python 3.7. So poetry update here does not get the bug-exposing update.

However users who are on recent python versions do get the update, and do experience errors eg see pappasam/jedi-language-server#296

A couple of meta-comments on this issue:

  • probably it's time to drop support for python 3.7 here: much of the world has already done so, and by continuing to support it pygls is not seeing updates that its users are seeing
  • it's another example where adding type annotations would have caught the bug before it ever happened

@dimbleby
Copy link
Contributor Author

the test-pyodide CI check seems to be broken independent of this fix

@tombh tombh self-requested a review November 26, 2023 13:24
@tombh tombh merged commit a590d30 into openlawlibrary:main Nov 26, 2023
16 of 17 checks passed
@tombh
Copy link
Collaborator

tombh commented Nov 26, 2023

Thank you.

The only reason we were holding back on dropping Python 3.7 was because VSCode promises 3 months of EOL support for each version. I think September 27th was the last day of that. So I'll submit a PR to drop 3.7.

Also, I think it might be worth disabling the Pyodide test for now. We've got some ideas to fix it. I wonder if it can just made to not fail the build.

@dimbleby
Copy link
Contributor Author

Thanks.

If it is any trouble to this project to support python 3.7, then imo vscode's promises are not pygls's problem!

As the linked comment says: the implication of dropping python 3.7 support is anyway only that vscode would decline to take updates until those three months passed. Which presumably is what they are doing already with every other dependency that has dropped python 3.7 support.

Anyway I guess it's all academic now that everyone is happy to drop python 3.7.

It also occurred to me that this bug provided a (confusing) datapoint in the discussion about bounds on dependencies

  • clearly it is an example of a dependency releasing a breaking change, causing non-obvious bugs for users
  • on the other hand cattrs is not a direct dependency, and does not use semantic versioning: so it is unlikely that pygls would ever have pinned it in a way that avoided this

@tombh
Copy link
Collaborator

tombh commented Nov 26, 2023

Indeed, it didn't come up as a problem holding off on Python 3.7 deprecation, so I think it's all good. We'd obviously have to discuss it further if it had been.

Oh yeah, BTW, I totally agree about how this PR wouldn't have been needed if we'd better typing. My day job is in Golang and it feels a bit archaic coming back to this largely untyped codebase.

Thanks for mentioning that this is a datapoint in the discussion about bounds on dependencies. So even though cattrs doesn't follow semantic versioning would there still not be a pinnable version before which the bug was introduced. That way at least poetry.lock could be used as an at least "blessed" (rather than enforced) snapshot of working versions.

BTW we do actually have a Python version matrix setup on Github Actions, so in theory, if our poetry.lock did have the problematic version (as that is what is used to run the tests), then should we have seen the bug you found in the higher Python version matrix test runs?

@dimbleby
Copy link
Contributor Author

dimbleby commented Nov 26, 2023

poetry prefers to find a single version of all dependencies compatible with all supported python versions. So even though you have a 3.8 python in your CI you are still installing the cattrs version from poetry.lock, which is a version that is compatible with python 3.7.

If you dropped python 3.7 support before merging this fix (and remembered to poetry update) then you should have seen CI failures.

the "blessed" versions in poetry.lock do not help anyone outside of pygls developers: only the constraints from pyproject.toml make it to pypi, so everyone else who installs or uses pygls knows nothing about poetry.lock.

(This is an argument for not using poetry.lock in CI. The trade is: reproducible pipelines, vs seeing the bugs that your users will see)

@tombh
Copy link
Collaborator

tombh commented Nov 26, 2023

poetry prefers to find a single version of all dependencies compatible with all supported python versions

Ahh yes of course, good point.

I like that framing of the lock/no-lock tradeoffs. Maybe we could have another CI step that always runs against latest versions?

@dimbleby
Copy link
Contributor Author

Maybe we could have another CI step that always runs against latest versions?

That's not a thing I've seen done; but there's a logic to it. Depends whether you think it's worth the trouble I guess.

@dimbleby
Copy link
Contributor Author

in fact cattrs is imported directly by pygls eg here so it should all along have been declared as a dependency in pyproject.toml

it still is entirely unclear what upper bound, if any, pygls ought to put on that dependency.

@fxp0
Copy link

fxp0 commented Nov 29, 2023

Is there any ETA for the next release with this fix?

@tombh tombh mentioned this pull request Nov 29, 2023
@tombh
Copy link
Collaborator

tombh commented Nov 29, 2023

We can release now. It's just nice to have fixes live on the main branch a while to test the waters.

PR for the release: #419

@tombh
Copy link
Collaborator

tombh commented Nov 30, 2023

Released ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants